Can you put this outside of the defined(WOLFSSL_ESPIDF)? That way other platforms could use it as well.

Might want to call the function ShowExtendedSystemInfo since you're printing out clock speeds and capabilities of the part as well.

Both good suggestions. Function is now ShowExtendedSystemInfo() and a bit more friendly to other platforms.

You should use a macro to print the lines. Then have a section above like so:

#if defined(WOLFSSL_ESPIDF)
#define VERSION_PRINTF(...) ESP_LOGI(TAG, ...)
#else
#define VERSION_PRINTF(...)
// or #warn "No printing capability
#endif

I wouldn't be surprised if we already have a macro like this somewhere

We may, but I put in a local VERSION_PRINTF

This stuff would be great in a general use case, for example if we compile with '--enable-debug' it would be nice to see this printed

Putting this stuff in a separate function like 'gitVersionInfo' might be better organized.

I think the end user can choose the --enable-debug option in their code around their call to ShowExtendedSystemInfo() .

I did however put things into separate functions for readability. (e.g. ShowExtendedSystemInfo_git())

Maybe then you could put the ESP32 stuff in a separate function that is guarded by WOLFSSL_ESPIDF?

Yup: see ShowExtendedSystemInfo_platform_espressif()

Shouldn't the definition of this come from configure.ac (i.e.: WOLFSSL_LIBRARY_VERSION)?

oh yes, good catch.

This needs "{}" for instance if we have in code:

if(false) WOLFSSL_VERSION_PRINTF("stuff");

then, that would always print a newline.

ah yes, good catch.

This has been implemented. Let me know if any other changes are needed.

This seems error prone when new parts come out we would have to be aware and add it to the list (not to mention propagate to customers). Instead, you should have an IF block on parts that do support ESP32WROOM32_CRYPT.

agreed. changed logic to known supported targets, everything else is an error

Don't put in unnecessary conditions. When we add support we can add those targets.

agreed. changed logic to known supported targets, everything else is an error

Negate this condition around so there is no empty block

well, no... it was actually a TODO. I instead put in a HWM from stack allocation address.

Remove extra empty line

done

Remove extra empty line

done

Remove extra empty line

done

Remove extra empty lines

done

Don't need this block

agreed. changed logic to known supported targets, everything else is an error

Adding a new files requires updating all the other build platforms like CMake, Visual Studio projects, etc. Please don't do this action unless a new file is absolutely required.

This shouldn't be a reason not to separate out functionality into smaller files. Our files are too big as it is. Instead, maybe update the other build platforms.

This is very ESP centric

Indeed this is very ESP centric at the moment, as I've been doing mostly Espressif development. My plan is to have an infrastructure in place to allow others to add their own platform-specific details as needed & desired. When doing so, and only as needed, the respective build & make platforms could be updated.

Although Espressif-specific now, I do see there being significant value in having a template of common system information for all platforms and all products. That would seem to be a valuable resource for customers to provide when asking for support.

It should be in the ESP specific code or even the ESP port code

I see your point here on the Espressif-specific details. I envisioned all of the "hardware / configuration overview" details being in a single file. As hardware typically has a version as well, that's another reason I was thinking of the version.c as a home. for this information.

If instead we had something way down in the ./port/expressif, what structure would you use? Perhaps a hardware specific ShowHardwareConfiguration() that is implemented for each platform, then called as appropriate from a single function elsewhere?

don't [add version.c] unless a new file is absolutely required

It was quite a learning curve to tease that file into the builds, so I definitely respect that comment. Still, iIntuitively for me, it seems to make sense to have this information in the version.c file. That seems to be a universal file & the most global across all wolfSSL libraries.

Why not extend the existing HAVE_WC_INTROSPECTION?

This is an excellent question. Although similar in concept, some of the information is mutually exclusive from introspection. @douzzer pointed out that: "HAVE_WC_INTROSPECTION requires that the configuration and build time artifacts, particularly the date/time of build and git parameters, be excluded from the build". For example: some of the information such as git hash & date may be updated, but the binary build itself should not actually change.

Given this, if you still feel strongly that I should not add the version.c, I'm happy to implement this functionality elsewhere. Any preference? Perhaps wolfcrypt/logging.c would be a appropriate? I certainly agree with @bandi13 about the big files. I'm in favor of more specific, smaller files when possible.

These config.h and settings.h go in the .c, not the .h.

I added the config.h and settings.h using the model from ssl.h; in particular I needed to include visibility.h in order to use the WOLFSSL_API macro, without which, the linker could not find ShowExtendedSystemInfo(). Is there a preferred way of doing this?

The __cplusplus logic only goes in the .h. It is already there, just remove in .c.

You probably don't need this as I don't think you use NO_VERSION_EXTENDED_INFO anywhere. Better to have the affirmative form of options, so that we can avoid things like #ifndef NO_VERSION_ to check that the extended info exists.